From 5260b3a748d7a30148b47ff0f273bac885c50c17 Mon Sep 17 00:00:00 2001 From: =?utf8?q?=D0=A1=D0=BA=D0=BE=D0=B2=D0=BE=D1=80=D0=BE=D0=B4=D0=B0=20?= =?utf8?q?=D0=9D=D0=B8=D0=BA=D0=B8=D1=82=D0=B0=20=D0=90=D0=BD=D0=B4=D1=80?= =?utf8?q?=D0=B5=D0=B5=D0=B2=D0=B8=D1=87?= Date: Fri, 7 Nov 2025 11:50:57 -0300 Subject: [PATCH] [PATCH] src,lib: refactor unsafe buffer creation to remove zero-fill toggle This removes the zero-fill toggle mechanism that allowed JavaScript to control ArrayBuffer initialization via shared memory. Instead, unsafe buffer creation now uses a dedicated C++ API. Refs: https://hackerone.com/reports/3405778 Co-Authored-By: Rafael Gonzaga Co-Authored-By: Joyee Cheung Signed-off-by: RafaelGSS PR-URL: https://github.com/nodejs-private/node-private/pull/759 Backport-PR-URL: https://github.com/nodejs-private/node-private/pull/799 CVE-ID: CVE-2025-55131 Gbp-Pq: Topic sec Gbp-Pq: Name 38-refactor-unsafe-buffer-creation-to-remove-zero-fill-toggle.patch --- deps/v8/include/v8-array-buffer.h | 7 +++ deps/v8/src/api/api.cc | 17 ++++++ lib/internal/buffer.js | 23 ++------ lib/internal/process/pre_execution.js | 2 - src/api/environment.cc | 3 +- src/node_buffer.cc | 84 ++++++++++++++++----------- 6 files changed, 82 insertions(+), 54 deletions(-) diff --git a/deps/v8/include/v8-array-buffer.h b/deps/v8/include/v8-array-buffer.h index 804fc42c4..e03ed1a6f 100644 --- a/deps/v8/include/v8-array-buffer.h +++ b/deps/v8/include/v8-array-buffer.h @@ -244,6 +244,13 @@ class V8_EXPORT ArrayBuffer : public Object { */ static std::unique_ptr NewBackingStore(Isolate* isolate, size_t byte_length); + /** + * Returns a new standalone BackingStore with uninitialized memory and + * return nullptr on failure. + * This variant is for not breaking ABI on Node.js LTS. DO NOT USE. + */ + static std::unique_ptr NewBackingStoreForNodeLTS( + Isolate* isolate, size_t byte_length); /** * Returns a new standalone BackingStore that takes over the ownership of * the given buffer. The destructor of the BackingStore invokes the given diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc index a06394e6c..da0c960f9 100644 --- a/deps/v8/src/api/api.cc +++ b/deps/v8/src/api/api.cc @@ -8743,6 +8743,23 @@ std::unique_ptr v8::ArrayBuffer::NewBackingStore( static_cast(backing_store.release())); } +std::unique_ptr v8::ArrayBuffer::NewBackingStoreForNodeLTS( + Isolate* v8_isolate, size_t byte_length) { + i::Isolate* i_isolate = reinterpret_cast(v8_isolate); + API_RCS_SCOPE(i_isolate, ArrayBuffer, NewBackingStore); + CHECK_LE(byte_length, i::JSArrayBuffer::kMaxByteLength); + ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate); + std::unique_ptr backing_store = + i::BackingStore::Allocate(i_isolate, byte_length, + i::SharedFlag::kNotShared, + i::InitializedFlag::kUninitialized); + if (!backing_store) { + return nullptr; + } + return std::unique_ptr( + static_cast(backing_store.release())); +} + std::unique_ptr v8::ArrayBuffer::NewBackingStore( void* data, size_t byte_length, v8::BackingStore::DeleterCallback deleter, void* deleter_data) { diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js index fbe9de249..23df382f1 100644 --- a/lib/internal/buffer.js +++ b/lib/internal/buffer.js @@ -30,7 +30,7 @@ const { hexWrite, ucs2Write, utf8Write, - getZeroFillToggle, + createUnsafeArrayBuffer, } = internalBinding('buffer'); const { @@ -1053,26 +1053,14 @@ function markAsUntransferable(obj) { obj[untransferable_object_private_symbol] = true; } -// A toggle used to access the zero fill setting of the array buffer allocator -// in C++. -// |zeroFill| can be undefined when running inside an isolate where we -// do not own the ArrayBuffer allocator. Zero fill is always on in that case. -let zeroFill = getZeroFillToggle(); function createUnsafeBuffer(size) { - zeroFill[0] = 0; - try { + if (size <= 64) { + // Allocated in heap, doesn't call backing store anyway + // This is the same that the old impl did implicitly, but explicit now return new FastBuffer(size); - } finally { - zeroFill[0] = 1; } -} -// The connection between the JS land zero fill toggle and the -// C++ one in the NodeArrayBufferAllocator gets lost if the toggle -// is deserialized from the snapshot, because V8 owns the underlying -// memory of this toggle. This resets the connection. -function reconnectZeroFillToggle() { - zeroFill = getZeroFillToggle(); + return new FastBuffer(createUnsafeArrayBuffer(size)); } module.exports = { @@ -1082,5 +1070,4 @@ module.exports = { createUnsafeBuffer, readUInt16BE, readUInt32BE, - reconnectZeroFillToggle, }; diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index 0bbabb80c..96bbfb4c3 100644 --- a/lib/internal/process/pre_execution.js +++ b/lib/internal/process/pre_execution.js @@ -24,7 +24,6 @@ const { refreshOptions, getEmbedderOptions, } = require('internal/options'); -const { reconnectZeroFillToggle } = require('internal/buffer'); const { exposeInterface, exposeLazyInterfaces, @@ -98,7 +97,6 @@ function prepareExecution(options) { const { expandArgv1, initializeModules, isMainThread } = options; refreshRuntimeOptions(); - reconnectZeroFillToggle(); // Patch the process object and get the resolved main entry point. const mainEntry = patchProcessObject(expandArgv1); diff --git a/src/api/environment.cc b/src/api/environment.cc index 46106fa94..dd55f5569 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -107,8 +107,9 @@ void* NodeArrayBufferAllocator::Allocate(size_t size) { ret = allocator_->Allocate(size); else ret = allocator_->AllocateUninitialized(size); - if (LIKELY(ret != nullptr)) + if (ret != nullptr) [[likely]] { total_mem_usage_.fetch_add(size, std::memory_order_relaxed); + } return ret; } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index d438b2bbd..803233ef4 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -75,7 +75,6 @@ using v8::Object; using v8::SharedArrayBuffer; using v8::String; using v8::Uint32; -using v8::Uint32Array; using v8::Uint8Array; using v8::Value; @@ -1177,35 +1176,6 @@ void SetBufferPrototype(const FunctionCallbackInfo& args) { realm->set_buffer_prototype_object(proto); } -void GetZeroFillToggle(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - NodeArrayBufferAllocator* allocator = env->isolate_data()->node_allocator(); - Local ab; - // It can be a nullptr when running inside an isolate where we - // do not own the ArrayBuffer allocator. - if (allocator == nullptr) { - // Create a dummy Uint32Array - the JS land can only toggle the C++ land - // setting when the allocator uses our toggle. With this the toggle in JS - // land results in no-ops. - ab = ArrayBuffer::New(env->isolate(), sizeof(uint32_t)); - } else { - uint32_t* zero_fill_field = allocator->zero_fill_field(); - std::unique_ptr backing = - ArrayBuffer::NewBackingStore(zero_fill_field, - sizeof(*zero_fill_field), - [](void*, size_t, void*) {}, - nullptr); - ab = ArrayBuffer::New(env->isolate(), std::move(backing)); - } - - ab->SetPrivate( - env->context(), - env->untransferable_object_private_symbol(), - True(env->isolate())).Check(); - - args.GetReturnValue().Set(Uint32Array::New(ab, 0, 1)); -} - void DetachArrayBuffer(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (args[0]->IsArrayBuffer()) { @@ -1397,6 +1367,54 @@ void CopyArrayBuffer(const FunctionCallbackInfo& args) { memcpy(dest, src, bytes_to_copy); } +// Converts a number parameter to size_t suitable for ArrayBuffer sizes +// Could be larger than uint32_t +// See v8::internal::TryNumberToSize and v8::internal::NumberToSize +inline size_t CheckNumberToSize(Local number) { + CHECK(number->IsNumber()); + double value = number.As()->Value(); + // See v8::internal::TryNumberToSize on this (and on < comparison) + double maxSize = static_cast(std::numeric_limits::max()); + CHECK(value >= 0 && value < maxSize); + size_t size = static_cast(value); +#ifdef V8_ENABLE_SANDBOX + CHECK_LE(size, kMaxSafeBufferSizeForSandbox); +#endif + return size; +} + +void CreateUnsafeArrayBuffer(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + if (args.Length() != 1) { + env->ThrowRangeError("Invalid array buffer length"); + return; + } + + size_t size = CheckNumberToSize(args[0]); + + Isolate* isolate = env->isolate(); + + Local buf; + + NodeArrayBufferAllocator* allocator = env->isolate_data()->node_allocator(); + // 0-length, or zero-fill flag is set, or building snapshot + if (size == 0 || per_process::cli_options->zero_fill_all_buffers || + allocator == nullptr) { + buf = ArrayBuffer::New(isolate, size); + } else { + std::unique_ptr store = + ArrayBuffer::NewBackingStoreForNodeLTS(isolate, size); + if (!store) { + // This slightly differs from the old behavior, + // as in v8 that's a RangeError, and this is an Error with code + return env->ThrowRangeError("Array buffer allocation failed"); + } + buf = ArrayBuffer::New(isolate, std::move(store)); + } + + args.GetReturnValue().Set(buf); +} + void Initialize(Local target, Local unused, Local context, @@ -1428,6 +1446,8 @@ void Initialize(Local target, SetMethod(context, target, "detachArrayBuffer", DetachArrayBuffer); SetMethod(context, target, "copyArrayBuffer", CopyArrayBuffer); + SetMethodNoSideEffect( + context, target, "createUnsafeArrayBuffer", CreateUnsafeArrayBuffer); SetMethod(context, target, "swap16", Swap16); SetMethod(context, target, "swap32", Swap32); @@ -1464,8 +1484,6 @@ void Initialize(Local target, SetMethod(context, target, "hexWrite", StringWrite); SetMethod(context, target, "ucs2Write", StringWrite); SetMethod(context, target, "utf8Write", StringWrite); - - SetMethod(context, target, "getZeroFillToggle", GetZeroFillToggle); } } // anonymous namespace @@ -1508,10 +1526,10 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(StringWrite); registry->Register(StringWrite); registry->Register(StringWrite); - registry->Register(GetZeroFillToggle); registry->Register(DetachArrayBuffer); registry->Register(CopyArrayBuffer); + registry->Register(CreateUnsafeArrayBuffer); registry->Register(Atob); registry->Register(Btoa); -- 2.30.2